feat(sqlsmith, deterministic-test): deterministic fuzz stability #7967

kwannoel · 2023-02-16T07:48:52Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

#7978

Adds 32 different sets of ddl + queries. This is run by deterministic test.
Daily cron will still run the usual seeded deterministic test.
PR and main workflow will run with the queries in freeze.zip in deterministic test environment.
Adds generate + run_pre_generated commands to sqlsmith.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features).
I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

My PR DOES NOT contain user-facing changes.

Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

Installation and deployment
Connector (sources & sinks)
SQL commands, functions, and operators
RisingWave cluster configuration changes
Other (please specify in the release note below)

Release note

src/tests/sqlsmith/tests/freeze/ddl.sql

codecov · 2023-02-16T11:42:34Z

Codecov Report

Merging #7967 (646de18) into main (1e0c0d2) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #7967      +/-   ##
==========================================
- Coverage   71.63%   71.63%   -0.01%     
==========================================
  Files        1132     1132              
  Lines      182213   182213              
==========================================
- Hits       130526   130523       -3     
- Misses      51687    51690       +3

Flag	Coverage Δ
rust	`71.63% <ø> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/object_store/src/object/mem.rs	`86.87% <0.00%> (-0.39%)`	⬇️
src/storage/src/hummock/compactor/iterator.rs	`96.40% <0.00%> (-0.28%)`	⬇️
src/common/src/types/ordered_float.rs	`30.87% <0.00%> (-0.20%)`	⬇️
src/storage/src/hummock/sstable_store.rs	`64.77% <0.00%> (-0.16%)`	⬇️
src/storage/src/hummock/compactor/mod.rs	`80.88% <0.00%> (+0.19%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/tests/sqlsmith/src/runner.rs

kwannoel · 2023-02-20T04:50:19Z

Bump, can someone help to review this? Thanks!

wangrunji0408 · 2023-02-20T06:41:30Z

src/tests/sqlsmith/src/runner.rs

+        Ok(_) => tracing::info!("successfully wrote to {}", path.display()),
+    }
+}
+
 /// e2e test runner for sqlsmith
 pub async fn run(client: &tokio_postgres::Client, testdata: &str, count: usize) {
    let mut rng = rand::rngs::SmallRng::from_entropy();


Just came up with an idea: Can we make queries deterministic if we provide a fixed seed for this RNG?

The current from_entropy reads from the global RNG in the simulation test. If the execution path before this point changes (likely to happen when RW code developed), the initial state of this RNG changes as well. I guess that's why queries are not stable across PRs. 👀

Okay let's try it first before this PR, that is simple change.

kwannoel · 2023-02-20T15:43:10Z

Made redundant by #8047? Let's observe deterministic-test fuzzing's stability for the next week or so...

kwannoel · 2023-02-21T03:27:28Z

obsolete after #8068

fuyufjh · 2023-02-23T03:59:10Z

Another advantage of a fixed test set is that it allows us to ignore some cases. I guess this might be very useful, because there are lots of corner cases in SQL and expressions and most of these corner cases are unlikely to happen in real-world use cases. Instead of fixing them immediately, I tend to record first and mark them as low-priority bugs.

Also, if we have the ability to ignore/disable a test case temporarily, it would be much easier to enable more features in SQLSmith because it won't be blocked by fixing these bugs. You can simply create these GitHub issues and ask the assignee to re-enable these ignored case once the bug gets fixed.

BTW, if you don't like to check in these random cases, we could open a separated GitHub repo to store them. BuildKite allows us to do it easily.

What do you think? @kwannoel @lmatz

lmatz · 2023-02-23T04:31:55Z

Instead of fixing them immediately, I tend to record first and mark them as low-priority bugs.
we have the ability to ignore/disable a test case temporarily,

Both agree

what about making fuzz testing a separate pipeline, and allowing merging a PR even when it fails?
But failed PR should open/link an issue of this failed test case

kwannoel · 2023-02-23T09:31:55Z

Agree to both as well.

what about making fuzz testing a separate pipeline, and allowing merging a PR even when it fails?
But failed PR should open/link an issue of this failed test case.

I think we should block PR still if fuzz testing with pre-generated queries fail. Property of pre-generated query set is that when they were generated, either:
A) Some queries are disabled because of bugs. i.e. these are commented out in the generated test set and won't be ran.
B) The rest should pass.

If any query fails then, it is failing from B). This should indicate a regression caused by the PR. So the PR should not be merged still.

kwannoel · 2023-02-23T13:27:25Z

PTAL, ready for review @lmatz @jon-chuang @fuyufjh @wangrunji0408.

Also, if we have the ability to ignore/disable a test case temporarily, it would be much easier to enable more features in SQLSmith because it won't be blocked by fixing these bugs.

Will re-enable inserts and try this approach in a separate PR.

fuyufjh · 2023-02-24T03:20:47Z

src/tests/sqlsmith/src/runner.rs

+}
+
+/// e2e query generator
+/// The goal is to generate NON-FAILING queries.


If we can support ignoring cases, this seems not necessary any more. In my mind, we can generate cases regardless of success of failure, and then mark to ignore thse unsupported ones.

Agree, plan to work on this in a separate PR.

ci/scripts/deterministic-e2e-test.sh

kwannoel added 7 commits February 16, 2023 10:53

consolidate error reason for easier error reporting

b6d1ab3

add query generator and runner for pre-generated queries

afb5289

add cli interfaces

0eb23a0

write generated query to file

e24aa2a

add outdir

6f50ba5

format output sql

5906a37

fix run pre-gen logic

21bfa4f

github-actions bot added the type/feature label Feb 16, 2023

kwannoel added 3 commits February 16, 2023 16:07

fix

8d2053f

update ci to use pre-generated queries for PRs and main

43efc86

add tracing

a7fd394

kwannoel force-pushed the deterministic-fuzz-stability branch from 521b9af to a7fd394 Compare February 16, 2023 08:27

kwannoel commented Feb 16, 2023

View reviewed changes

src/tests/sqlsmith/tests/freeze/ddl.sql Outdated Show resolved Hide resolved

kwannoel marked this pull request as ready for review February 16, 2023 08:29

kwannoel force-pushed the deterministic-fuzz-stability branch from 8b44f88 to a7fd394 Compare February 16, 2023 08:30

kwannoel added 5 commits February 16, 2023 17:35

add script to generate N deterministic fuzzing queries

c6c4d83

avoid recompile

7f471e1

do parallel gen via deterministic simulation

5e17adb

gen

95223ae

fmt

a67fb87

kwannoel force-pushed the deterministic-fuzz-stability branch from 14c1980 to a67fb87 Compare February 16, 2023 10:51

add explanation

19349b9

kwannoel mentioned this pull request Feb 16, 2023

Tracking: deterministic fuzzing test stability #7978

Closed

3 tasks

fix

4c67437

kwannoel requested review from wangrunji0408, fuyufjh, jon-chuang and lmatz February 16, 2023 14:35

kwannoel commented Feb 17, 2023

View reviewed changes

src/tests/sqlsmith/src/runner.rs Outdated Show resolved Hide resolved

kwannoel commented Feb 17, 2023

View reviewed changes

src/tests/sqlsmith/src/runner.rs Outdated Show resolved Hide resolved

Use query instead of execute for queries expected to return results

e042515

wangrunji0408 reviewed Feb 20, 2023

View reviewed changes

kwannoel closed this Feb 21, 2023

kwannoel reopened this Feb 23, 2023

kwannoel mentioned this pull request Feb 23, 2023

bug(sqlsmith): sqlsmith running in madsim is still not reproducible #8152

Closed

kwannoel added 4 commits February 23, 2023 18:20

Merge remote-tracking branch 'origin' into deterministic-fuzz-stability

905ad3e

fix

e2fc0d4

update freeze.zip

b85274e

fix

d744850

kwannoel requested a review from wangrunji0408 February 23, 2023 13:23

fuyufjh reviewed Feb 24, 2023

View reviewed changes

ci/scripts/deterministic-e2e-test.sh Outdated Show resolved Hide resolved

kwannoel added 2 commits February 24, 2023 11:44

use text

e6bcfec

ignore sqlsmith artifacts in typochecker

9a292c3

fuyufjh approved these changes Feb 24, 2023

View reviewed changes

kwannoel added the mergify/can-merge label Feb 24, 2023

Merge branch 'main' into deterministic-fuzz-stability

646de18

mergify bot merged commit d257b5f into main Feb 24, 2023

mergify bot deleted the deterministic-fuzz-stability branch February 24, 2023 05:04

This was referenced Feb 28, 2023

fix(ci): add missing sqlsmith queries #8214

Merged

sqlsmith: generating a snapshot #8220

Closed

deterministic fuzzing test: stability #7901

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sqlsmith, deterministic-test): deterministic fuzz stability #7967

feat(sqlsmith, deterministic-test): deterministic fuzz stability #7967

kwannoel commented Feb 16, 2023 •

edited

Loading

codecov bot commented Feb 16, 2023 •

edited

Loading

kwannoel commented Feb 20, 2023

wangrunji0408 Feb 20, 2023

kwannoel Feb 20, 2023

kwannoel Feb 20, 2023

kwannoel commented Feb 20, 2023

kwannoel commented Feb 21, 2023

fuyufjh commented Feb 23, 2023 •

edited

Loading

lmatz commented Feb 23, 2023

kwannoel commented Feb 23, 2023 •

edited

Loading

kwannoel commented Feb 23, 2023 •

edited

Loading

fuyufjh Feb 24, 2023

kwannoel Feb 24, 2023

feat(sqlsmith, deterministic-test): deterministic fuzz stability #7967

feat(sqlsmith, deterministic-test): deterministic fuzz stability #7967

Conversation

kwannoel commented Feb 16, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Types of user-facing changes

Release note

codecov bot commented Feb 16, 2023 • edited Loading

Codecov Report

kwannoel commented Feb 20, 2023

wangrunji0408 Feb 20, 2023

Choose a reason for hiding this comment

kwannoel Feb 20, 2023

Choose a reason for hiding this comment

kwannoel Feb 20, 2023

Choose a reason for hiding this comment

kwannoel commented Feb 20, 2023

kwannoel commented Feb 21, 2023

fuyufjh commented Feb 23, 2023 • edited Loading

lmatz commented Feb 23, 2023

kwannoel commented Feb 23, 2023 • edited Loading

kwannoel commented Feb 23, 2023 • edited Loading

fuyufjh Feb 24, 2023

Choose a reason for hiding this comment

kwannoel Feb 24, 2023

Choose a reason for hiding this comment

kwannoel commented Feb 16, 2023 •

edited

Loading

codecov bot commented Feb 16, 2023 •

edited

Loading

fuyufjh commented Feb 23, 2023 •

edited

Loading

kwannoel commented Feb 23, 2023 •

edited

Loading

kwannoel commented Feb 23, 2023 •

edited

Loading